fix: propagate null bitmap in evaluate_map_to_struct by momcilomrk-db · Pull Request #2419 · delta-io/delta-kernel-rs

momcilomrk-db · 2026-04-17T17:05:56Z

What changes are proposed in this pull request?

evaluate_map_to_struct creates the output StructArray with None as the null buffer, even when the input MapArray has null rows. This causes Arrow validation to reject the array with "Found unmasked nulls for non-nullable StructArray field" when any output field is non-nullable.

This manifests during checkpoint creation for partitioned tables where:

A partition column is declared NOT NULL in the table schema
delta.checkpoint.writeStatsAsStruct = true
The delta log contains non-add actions (remove/metadata/protocol) where add is null

The COALESCE(partitionValues_parsed, MAP_TO_STRUCT(partitionValues)) expression evaluates MAP_TO_STRUCT for all rows. For rows where the map is null, child builders get null values appended but the output struct has no null mask to cover them.

Context: MapToStruct was introduced in #1895 and used for partitionValues_parsed in #1932.

How was this change tested?

When the input MapArray has null rows (e.g. for remove/metadata/protocol actions where the parent `add` struct is null), evaluate_map_to_struct appends null values to all child field builders but creates the output StructArray with no null buffer (None). This causes Arrow validation to reject the array with "Found unmasked nulls for non-nullable StructArray field" when any output field is declared non-nullable. This manifests during checkpoint creation for partitioned tables with NOT NULL partition columns and delta.checkpoint.writeStatsAsStruct=true. The COALESCE(partitionValues_parsed, MAP_TO_STRUCT(partitionValues)) expression evaluates MAP_TO_STRUCT for all rows including those where the map is null, triggering the validation error. The fix propagates the input MapArray's null bitmap to the output StructArray, matching the pattern established in PR delta-io#1645 for nested transform expressions. Co-authored-by: Isaac

scovich · 2026-04-17T18:50:09Z

@momcilomrk-db -- please add unit tests for this change. Clearly we have a gap in coverage.

codecov · 2026-04-17T18:55:06Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.34%. Comparing base (6639d4e) to head (0fde572).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #2419    +/-   ##
========================================
  Coverage   88.34%   88.34%            
========================================
  Files         171      171            
  Lines       56696    56983   +287     
  Branches    56696    56983   +287     
========================================
+ Hits        50087    50341   +254     
- Misses       4695     4714    +19     
- Partials     1914     1928    +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dengsh12

LGTM, but please add tests for the case you mentioned in description, thanks!

scottsand-db · 2026-04-17T22:12:54Z

        arrow_fields.into(),
        output_columns,
-        None,
+        map_array.nulls().cloned(),


Please add a comment.

Before: This code was None and it was so obviously correct that it wasn't worth having a comment about why it is None.

Now: The code is map_array.nulls().cloned() and, since I don't see any comment, apparently it is obvious why this should be map_array.nulls().cloned().

Yet -- this was a bug. Clearly it is not obvious.

Please add a detailed comment explaining why, what happens when it is present, what happens if it was NOT present, etc.

We need to remind ourselves in the future (and perhaps in other similar areas of the code) why this value MUST be set.

@dengsh12 and @DrakeLin -- Remember to raise the bar on PR reviews and aim for excellent code clariity

Good point! But in this special case ... I feel like reserving map_array.nulls().cloned() is the straightforward way(extract things from the MapArray and retain the nulls), perhaps we should add comments if we set it to None(uncommon since dropping the nullabilities)? Let's add a comment stating the corner case, and in the future claude would know this corner case exists

I agree I think it makes sense to add detailed comments now more than before since Claude can pick it up and not create the same bug in the future.

For example I asked the claude to look at the previous PRs (to find when the bug was introduced), and it also found this related issue #1645 which seems to be the same None pattern. If there was a comment maybe it would pick it up in this case as well.

scottsand-db · 2026-04-17T22:13:29Z

Please do not merge until comment above is addressed

Add three tests covering the null bitmap fix in evaluate_map_to_struct: 1. test_map_to_struct_propagates_null_bitmap_for_non_nullable_fields: Direct test -- null map rows with non-nullable output fields. Verifies the output struct is null where the input map is null, preventing "Found unmasked nulls for non-nullable StructArray field" errors. 2. test_coalesce_map_to_struct_with_null_map_non_nullable_fields: Simulates the checkpoint expression COALESCE(partitionValues_parsed, MAP_TO_STRUCT(partitionValues)) with a null map row and NOT NULL partition column. This is the exact pattern that fails during checkpoint creation. 3. test_map_to_struct_all_null_maps_with_non_nullable_fields: Edge case where every map row is null. Verifies the output struct is entirely null despite non-nullable child fields. Co-authored-by: Isaac

momcilomrk-db · 2026-04-20T09:52:51Z

Addressed the comments.

I added the tests you can review it @DrakeLin

DrakeLin

Can we use rstest for the first 2 test functions?

  #[rstest]
  #[case::mixed_nulls(
      vec![Some(vec![("k", "v")]), None, Some(vec![("k",
  "x")])],                                                      
      vec![true, false, true],
  )]                                                            
  #[case::all_nulls(                                          
      vec![None, None],
      vec![false, false],                                       
  )]
  fn test_map_to_struct_null_propagation_with_non_nullable_field
  s(...)

DrakeLin · 2026-04-20T17:23:45Z


+    #[test]
+    fn test_map_to_struct_null_map_with_non_nullable_fields() {
+        use crate::arrow::array::{MapBuilder, StringBuilder};


Hoist up to test module. Repeated in all tests.

Thanks for the comments. Both are done. Previously I followed the pattern from the other Rust tests in that file. I removed use for other tests as well.

The rstest is a good approach I did it, thanks.

- Hoist MapBuilder and StringBuilder imports to module level (removes 6 inline use statements across existing and new tests) - Consolidate the two MAP_TO_STRUCT null propagation tests into a single parametrized rstest with mixed_nulls and all_nulls cases - Fix rustfmt formatting in test_coalesce_map_to_struct Co-authored-by: Isaac

github-actions Bot assigned momcilomrk-db Apr 17, 2026

DrakeLin self-requested a review April 17, 2026 18:16

DrakeLin approved these changes Apr 17, 2026

View reviewed changes

DrakeLin requested a review from dengsh12 April 17, 2026 19:05

dengsh12 approved these changes Apr 17, 2026

View reviewed changes

scottsand-db reviewed Apr 17, 2026

View reviewed changes

scottsand-db self-requested a review April 17, 2026 22:13

momcilomrk-db added 3 commits April 20, 2026 08:43

make test more consistent

3a303d0

add comment

9e14638

momcilomrk-db requested a review from DrakeLin April 20, 2026 09:53

DrakeLin reviewed Apr 20, 2026

View reviewed changes

DrakeLin enabled auto-merge April 21, 2026 23:18

DrakeLin added this pull request to the merge queue Apr 21, 2026

Merged via the queue into delta-io:main with commit 136e975 Apr 21, 2026
21 checks passed

DrakeLin mentioned this pull request Apr 21, 2026

fix: force partition fields nullable in partitionValues_parsed schema #2420

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: propagate null bitmap in evaluate_map_to_struct#2419

fix: propagate null bitmap in evaluate_map_to_struct#2419
DrakeLin merged 5 commits intodelta-io:mainfrom
momcilomrk-db:fix/map-to-struct-null-propagation

momcilomrk-db commented Apr 17, 2026 •

edited

Loading

Uh oh!

scovich commented Apr 17, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Apr 17, 2026 •

edited

Loading

Uh oh!

dengsh12 left a comment

Uh oh!

scottsand-db Apr 17, 2026

Uh oh!

dengsh12 Apr 17, 2026 •

edited

Loading

Uh oh!

momcilomrk-db Apr 20, 2026 •

edited

Loading

Uh oh!

scottsand-db commented Apr 17, 2026

Uh oh!

momcilomrk-db commented Apr 20, 2026

Uh oh!

DrakeLin left a comment

Uh oh!

DrakeLin Apr 20, 2026

Uh oh!

momcilomrk-db Apr 21, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

momcilomrk-db commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes are proposed in this pull request?

How was this change tested?

Uh oh!

scovich commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dengsh12 left a comment

Choose a reason for hiding this comment

Uh oh!

scottsand-db Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

dengsh12 Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

momcilomrk-db Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottsand-db commented Apr 17, 2026

Uh oh!

momcilomrk-db commented Apr 20, 2026

Uh oh!

DrakeLin left a comment

Choose a reason for hiding this comment

Uh oh!

DrakeLin Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

momcilomrk-db Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

momcilomrk-db commented Apr 17, 2026 •

edited

Loading

scovich commented Apr 17, 2026 •

edited

Loading

codecov Bot commented Apr 17, 2026 •

edited

Loading

dengsh12 Apr 17, 2026 •

edited

Loading

momcilomrk-db Apr 20, 2026 •

edited

Loading

momcilomrk-db Apr 21, 2026 •

edited

Loading